Feature: absorb spiffe-helper config into global kagenti-platform-config#437
Conversation
3552380 to
5d568ba
Compare
esnible
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-tested refactor that moves spiffe-helper-config from static templates to being derived from PlatformConfig (single source of truth, always-overwrite). The Helm install path is correct and verified end-to-end: defaults.spiffe.helperConfig → kagenti-platform-config → loader → controller ensureSpiffeHelperConfigMap → namespace CM → volume_builder mount. Helm values.yaml and the Go DefaultSpiffeHelperConfig fallback are byte-identical, the dead SpiffeHelperConf field is cleanly removed across webhook/injector, and the new unit tests (create/overwrite/idempotent/nil-fallback, hash-change, YAML round-trip, DeepCopy, template-exclusion) are thorough and assertive.
One non-blocking concern about the OpenShift kustomize install path — see the inline comment. Posting as COMMENT (not REQUEST_CHANGES) since this may be a non-issue if OpenShift is now Helm-only.
Author: r3v5 (CONTRIBUTOR — returning external)
Areas reviewed: Go controller/webhook, Helm values, kustomize config, e2e fixtures, unit tests
Agent/IDE config (.claude/.vscode): none (supply-chain gate clean)
Commits: 4, all signed-off, conventional format
CI: 15/16 green, E2E pending
Assisted-By: Claude Code
| @@ -12,4 +12,3 @@ resources: | |||
|
|
|||
| patches: | |||
| - path: patches/authbridge-keycloak.yaml | |||
There was a problem hiding this comment.
Possible regression on the OpenShift kustomize path. This PR also deletes patches/spiffe-helper-keycloak.yaml (and its reference here). That patch overrode the spiffe-helper jwt_audience from the public keycloak.localtest.me to the in-cluster keycloak-service.keycloak.svc:8080/realms/kagenti for OpenShift — mirroring the authbridge-keycloak.yaml patch that's kept right above this line.
The new single source of truth, kagenti-platform-config, is created only by the Helm chart (templates/manager/configmap-platform-defaults.yaml). The make deploy-openshift / kustomize build config/openshift path never creates it, so loader.go falls back to CompiledDefaults() → DefaultSpiffeHelperConfig, which hardcodes the public keycloak.localtest.me:8080 audience. That won't resolve in-cluster on OpenShift.
If OpenShift is still installable via this kustomize overlay, the in-cluster audience override appears lost. Two ways to resolve:
- add an OpenShift mechanism to set
spiffe.helperConfigwith the in-cluster audience (parallel to howauthbridge-keycloak.yamlsurvives), or - if OpenShift is now Helm-only, remove/deprecate the
config/openshiftoverlay so it isn't left in a broken state.
Could you confirm which install path OpenShift uses now?
|
|
||
| // DefaultSpiffeHelperConfig is the default helper.conf content for spiffe-helper. | ||
| // Keep in sync with charts/kagenti-operator/values.yaml defaults.spiffe.helperConfig. | ||
| const DefaultSpiffeHelperConfig = `agent_address = "/spiffe-workload-api/spire-agent.sock" |
There was a problem hiding this comment.
Nit: DefaultSpiffeHelperConfig is the compiled fallback used whenever kagenti-platform-config is absent, and it bakes in the public keycloak.localtest.me audience. That's fine as a dev default, but it's also the silent fallback for any deploy path that doesn't create the platform-config CM (see the kustomize/OpenShift comment). A one-line note here that prod/OpenShift must override this via PlatformConfig would make the failure mode less surprising.
pdettori
left a comment
There was a problem hiding this comment.
Reviewed the Go controller/webhook changes, Helm values+template, kustomize, and e2e fixtures. Two things I verified and consider correct (no action needed):
- Helm wiring:
defaults.spiffe.helperConfiginvalues.yamlis rendered intokagenti-platform-configviatemplates/manager/configmap-platform-defaults.yaml(.Values.defaults | toYaml). ✓ - Reconcile ordering:
ensureSpiffeHelperConfigMap(step 4.5b) runs beforeComputeConfigHash(step 5), so the hash reflects the freshly-written CM — no one-reconcile lag. ✓
One low-severity suggestion inline. (I'm intentionally not re-raising the OpenShift jwt_audience regression already noted in the existing review — that remains the more consequential open item.)
Assisted-By: Claude Code
| return fmt.Errorf("failed to check spiffe-helper-config in %s: %w", namespace, err) | ||
| } | ||
|
|
||
| if existing.Data["helper.conf"] == cfg.Spiffe.HelperConfig { |
There was a problem hiding this comment.
suggestion: The idempotent early-return skips label reconciliation. When existing.Data["helper.conf"] already matches the platform config, the function returns here — before the existing.Labels[LabelManagedBy] = LabelManagedByValue block just below.
This means a spiffe-helper-config CM left over from the old static-template/copy path (labeled app.kubernetes.io/managed-by: kustomize, or unlabeled) whose content happens to equal the new default gets adopted in content but never receives the kagenti.io/managed-by ownership marker this single-source-of-truth design relies on. The new "idempotent when content matches" test doesn't catch it because it pre-seeds the CM with the label.
Consider reconciling the label before the content-equality short-circuit (e.g. ensure/patch the label whenever it's missing, independent of the data comparison).
|
Addressing all three review comments: @esnible — OpenShift kustomize path (kustomization.yaml:14) @esnible — DefaultSpiffeHelperConfig note (defaults.go:10) @pdettori — Idempotent path skips label reconciliation (agentruntime_controller.go:1099) |
kevincogan
left a comment
There was a problem hiding this comment.
Nice work addressing the earlier review feedback. The label adoption fix and the jwt_audience documentation update both look good to me.
One remaining thing I would like to check before approving: ensureSpiffeHelperConfigMap() now seems to be on the critical path for keeping PlatformConfig as the source of truth for spiffe-helper-config. If that create/update/check fails, reconcile currently logs/emits an event but then continues into config hash calculation and workload config application.
Would it be safer to return and requeue here, similar to the SCC setup path, so we do not risk stamping a workload using a stale or missing spiffe-helper-config?
I may be missing a reason this is intentionally best-effort, but I think it would be good to either make this fail/requeue or explicitly document why continuing is safe.
66f6c74 to
e64b378
Compare
Remove spiffe-helper-config from templateConfigMapNames since it is now derived from PlatformConfig via ensureSpiffeHelperConfigMap. Make failure in ensureSpiffeHelperConfigMap set error status and requeue (30s), matching the SCC binding pattern, to prevent stamping workloads with a stale or missing spiffe-helper-config. Addresses review feedback from kevincogan on PR rossoctl#437. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Remove spiffe-helper-config from templateConfigMapNames since it is now derived from PlatformConfig via ensureSpiffeHelperConfigMap. Make failure in ensureSpiffeHelperConfigMap set error status and requeue (30s), matching the SCC binding pattern, to prevent stamping workloads with a stale or missing spiffe-helper-config. Addresses review feedback from kevincogan on PR rossoctl#437. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
28307e4 to
48ce0ef
Compare
Remove spiffe-helper-config from templateConfigMapNames since it is now derived from PlatformConfig via ensureSpiffeHelperConfigMap. Make failure in ensureSpiffeHelperConfigMap set error status and requeue (30s), matching the SCC binding pattern, to prevent stamping workloads with a stale or missing spiffe-helper-config. Addresses review feedback from kevincogan on PR rossoctl#437. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
48ce0ef to
d597776
Compare
|
Good catch — you're right, continuing past a failed Fixed in Also removed |
Review Follow-up: All Comments AddressedChecked the latest revision (commits 5–7) against the outstanding review feedback:
CI is all green (16/16 passing). Remaining blocker: the branch has merge conflicts with Assisted-By: Claude Code |
Add HelperConfig string field to SpiffeConfig struct so PlatformConfig can carry raw spiffe-helper helper.conf content. Wire default content into CompiledDefaults() and log truncated snippet on startup. RHAIENG-4939 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Add GetPlatformConfig to AgentRuntimeReconciler and ensureSpiffeHelperConfigMap() which creates/overwrites the spiffe-helper-config CM per namespace from PlatformConfig. Remove spiffe-helper-config from templateConfigMapNames (no longer template-copied). Include helperConfig in config hash so changes trigger rolling updates. RHAIENG-4939 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Webhook no longer reads spiffe-helper-config CM at admission time. Remove SpiffeHelperConf field from NamespaceConfig and ResolvedConfig structs. The controller now derives the CM from PlatformConfig; the volume mount still references the CM by name (unchanged). RHAIENG-4939 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Add spiffe section with helperConfig to Helm values.yaml under defaults. Delete static spiffe-helper-config.yaml template and OpenShift spiffe-helper-keycloak.yaml kustomize patch. Update kustomization references and e2e fixture comments. RHAIENG-4939 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Address PR review feedback: - Label reconciliation now runs before content comparison so leftover CMs from the old static-template path get their managed-by label adopted even when content already matches PlatformConfig. - Add prod/OpenShift override note to DefaultSpiffeHelperConfig. - Add test for label adoption on leftover CMs. RHAIENG-4939 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Wrap AgentRuntime CR apply in Eventually with 1m timeout and 5s poll, matching the retry pattern already used by authbridge and combined e2e tests. Fixes flaky webhook connection-refused race when the validation webhook TCP listener isn't accepting connections yet despite endpoint being registered. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
Remove spiffe-helper-config from templateConfigMapNames since it is now derived from PlatformConfig via ensureSpiffeHelperConfigMap. Make failure in ensureSpiffeHelperConfigMap set error status and requeue (30s), matching the SCC binding pattern, to prevent stamping workloads with a stale or missing spiffe-helper-config. Addresses review feedback from kevincogan on PR rossoctl#437. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
d597776 to
661dd7f
Compare
Merge conflicts have been resolved |
kevincogan
left a comment
There was a problem hiding this comment.
Thanks for the rebase and for addressing the previous feedback. The main blocker I raised looks fixed now: ensureSpiffeHelperConfigMap() failures update status and requeue instead of continuing and potentially stamping workloads with stale or missing helper config.
One remaining thing I think is worth tightening before approval: after ensureSpiffeHelperConfigMap() creates or updates spiffe-helper-config, ComputeConfigHash() is still called with the cached client. Since that read may come from the controller-runtime cache, the hash could be computed from stale or missing helper.conf immediately after the update.
Could we make this deterministic by either computing the hash with the uncached reader/APIReader, or requeueing after creating/updating spiffe-helper-config before applying the workload config?
After that, assuming the latest checks are green, I’d be comfortable approving.
…nfig write ensureSpiffeHelperConfigMap() creates/updates the CM via the API server, but ComputeConfigHash() was reading it back through the controller-runtime cache, which may not yet reflect the write. Switch to uncachedReader() so the hash is always computed from the authoritative state. Addresses review feedback from kevincogan on PR rossoctl#437. RHAIENG-4939 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
|
Hey @kevincogan ! I have addressed your comment. |
|
FYI, sharing the latest manual testing in local Kind cluster Verification: RHAIENG-4939 — Absorb spiffe-helper config into PlatformConfigDate: 2026-06-24 Environment
1. Create Kind cluster$ kind create cluster --name kagenti2. Build operator image with podman$ cd kagenti-operator/
$ make docker-build CONTAINER_TOOL=podman3. Load image into KindThe Makefile $ git rev-parse --short HEAD
661dd7f
$ podman tag localhost/controller:latest localhost/kagenti-operator:661dd7f
$ podman save localhost/kagenti-operator:661dd7f -o /tmp/kagenti-operator.tar
$ kind load image-archive /tmp/kagenti-operator.tar --name kagentiVerified image present on Kind node: $ podman exec kagenti-control-plane crictl images | grep kagenti4. Install cert-manager$ kubectl --context kind-kagenti apply -f https://github.com/cert-manager/cert-manager/releases/latest/download/cert-manager.yaml$ kubectl --context kind-kagenti wait --for=condition=Available -n cert-manager deployment/cert-manager-webhook --timeout=120s5. Install operator via Helm$ helm --kube-context kind-kagenti upgrade --install --create-namespace \
-n kagenti-system kagenti-operator ../charts/kagenti-operator \
--set controllerManager.container.image.repository=localhost/kagenti-operator \
--set controllerManager.container.image.tag=661dd7f$ kubectl --context kind-kagenti wait --for=condition=Available -n kagenti-system deployment/kagenti-controller-manager --timeout=120s6. Verification: PlatformConfig CM contains
|
| # | Check | Result |
|---|---|---|
| 1 | kagenti-platform-config CM contains spiffe.helperConfig |
PASS |
| 2 | Creating AgentRuntime produces spiffe-helper-config CM in agent namespace |
PASS |
| 3 | CM content matches PlatformConfig | PASS |
| 4 | Controller log confirms PlatformConfig derivation (not template copy) | PASS |
| 5 | Config-hash annotation applied to workload (enables rolling updates) | PASS |
| 6 | AgentRuntime status conditions healthy | PASS |
Overall: ALL CHECKS PASS
The spiffe-helper-config is correctly derived from PlatformConfig at runtime, no longer copied from static templates. Changes to defaults.spiffe.helperConfig in Helm values will propagate to all agent namespaces and trigger rolling updates via config-hash.
kevincogan
left a comment
There was a problem hiding this comment.
Thanks for the follow-up fix. I rechecked the latest revision against main, and my remaining concern looks addressed now: ComputeConfigHash() uses the uncached reader after ensureSpiffeHelperConfigMap(), so the hash should reflect the freshly written spiffe-helper-config.
The other items I was tracking also look covered, and the PlatformConfig source-of-truth design looks clean.
This looks good to me from my end.
pdettori
left a comment
There was a problem hiding this comment.
Approving — my prior feedback is fully addressed and conflicts are resolved.
- Label reconciliation suggestion:
ensureSpiffeHelperConfigMapnow reconciles thekagenti.io/managed-byownership label before the content-equality short-circuit, so a leftover CM with matching content but a missing/stale label still gets adopted. ✓ - Conflicts: rebased, branch is MERGEABLE. ✓
- Also confirmed @kevincogan's items: failure now requeues (
RequeueAfter: 30s) andComputeConfigHashuses the uncached reader, so the hash reflects the freshly-writtenspiffe-helper-config. ✓
All 16 checks green (E2E + Integration included). PlatformConfig source-of-truth design looks clean.
Non-blocking note for follow-up: deleting config/openshift/patches/spiffe-helper-keycloak.yaml is safe only if OpenShift is now Helm-only; if the kustomize overlay is still a supported install path, the in-cluster jwt_audience override falls back to the public dev default. This was @esnible's earlier (non-blocking) point and is documented in defaults.go now — flagging so it's not lost.
Assisted-By: Claude Code
Summary
HelperConfigfield toSpiffeConfigso PlatformConfig carries spiffe-helperhelper.confcontentspiffe-helper-configConfigMaps from PlatformConfig (always-overwrite, single source of truth)spiffe-helper-configfrom template-copy list — no longer blindly copied from kagenti-systemhelperConfigin config hash so changes trigger rolling updatesspiffe-helper-configCM at admission time (dead field removed)spiffesection to Helmvalues.yamlwith configurablehelperConfigTest plan
ensureSpiffeHelperConfigMap(create, overwrite, idempotent, nil fallback), config hash change detection, schema round-trip, and template exclusionkagenti-platform-configCM containsspiffe.helperConfigspiffe-helper-configCM in agent namespacehelperConfigwith sensible defaultshelperConfigtriggers rolling update via config hashCloses RHAIENG-4939
🤖 Generated with Claude Code